Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sure the session is committed only once per request #1783

Closed
wants to merge 2 commits into from
Closed

make sure the session is committed only once per request #1783

wants to merge 2 commits into from

Conversation

sadraskol
Copy link

When the session is queried by another filter through the request, any subsequent calls to jsp:include commits the session and fetches the session to do so. To avoid that, I forced commitSession to be called once. I ignore if this interacts with other parts of spring-session but it seems a reasonable solution.

You can find an example of this behavior in this small application: https://github.com/sadraskol/spring-session-jsp

solves #1424

@pivotal-issuemaster
Copy link

@sadraskol Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@sadraskol Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 29, 2021
@sadraskol
Copy link
Author

These changes are not working properly: since the session is "committed" once, any subsequent changes on the session are discarded. I don't think this is an expected behavior.
I attempted to adapt my changes, but there is one operation that I don't understand: why do we need to call clearRequestedSessionCache when committing the session? I don't quite get its purpose in the event. My mental model tells me that we could get rid of it.

I think @vpavic mentionned a concurrency bug in the ordering of this method. Has it anything to do with it?

@sadraskol
Copy link
Author

I implemented an alternative way to make it work : clear the cache only when the request is handled. I don't know if this works as intended though.

@vpavic vpavic self-requested a review March 3, 2021 16:39
@vpavic vpavic self-assigned this Mar 3, 2021
@sadraskol
Copy link
Author

Hi there! Any news on this issue? I would like to understand the problems we're facing to help out.

@quaff
Copy link
Contributor

quaff commented Sep 3, 2021

@sadraskol Could you test #1910 against your case?

Copy link
Contributor

@vpavic vpavic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was taking a closer look at this, but upon inspecting the Servlet spec on include dispatch, it appears to me that commit session shouldn't even be invoked there as it potentially writes headers and therefore violates the spec.

@rwinch Let's discuss this on Monday to decide what we do for 3.0. I'm inclined to merge #1909 instead.

@sadraskol
Copy link
Author

Sorry for my late response.
Indeed #1910 is much more clean than my PR in which i tried to have least side effect as possible.
I would like to see the test replicated to make sure it does work.

Unfortunately, I do not work in the company that encountered the problem... I'll forward them the PR and let them answer if they still have the issue.

@vpavic
Copy link
Contributor

vpavic commented Oct 19, 2022

Thanks for following up @sadraskol.

My previous comment was a bit outdated. In the end, we went with #1909 (see also #2194) and that is a part of 3.0.0-RC1 release yesterday. Those changes should consistently eliminate one round trip to datastore to fetch the session.

Regarding #1910, we intend to revisit that at some point, but we want to make sure that if changes to session do happen after the response has been committed (and that's certainly possible), they would still get persisted.

Finally, thanks for your contributions to this topic, even though this PR didn't get merged in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants